moves shred deduper inside the thread-pool #4179
Conversation
behzadnouri
force-pushed
the
retransmit-par-dedup
branch
2 times, most recently
from
94f54e5 to
6b5c516
Compare
turbine/src/retransmit_stage.rs
Outdated
| root_bank: &Bank, | ||
| cluster_nodes: &ClusterNodes<RetransmitStage>, | ||
| shred_deduper: &ShredDeduper<2>, |
There was a problem hiding this comment.
what is 2 here (or K in `ShredDeduper)? and would it make sense to make it a constant with a descriptive name?
There was a problem hiding this comment.
It is the number of hashers used in inner Deduper.
I added a default value, so that the call-sites do not have to repeat that. see last commit here: 47c84f7
There was a problem hiding this comment.
looks good but should it be a const default such as:
const DEDUP_HASHERS: usize = 2;
and then:
struct ShredDeduper<const K: usize = DEDUP_HASHERS> {
deduper: Deduper<K, /*shred:*/ [u8]>,
shred_id_filter: Deduper<K, (ShredId, /*0..MAX_DUPLICATE_COUNT:*/ usize)>,
}
just not a huge fan of standalone numbers in code if we can avoid it.
There was a problem hiding this comment.
I slightly prefer to keep it inline because it is more evident what the default is equal to (you don't need to jump elsewhere in the code to find that out).
Either way, ShredDeduper<K> definition is independent of this change. So I think it would be better to revise this in a separate PR if needed.
There was a problem hiding this comment.
ya i get the not wanting to jump around in code. just prefer the naming clarity over the cost to jump. but ya can be done in separate PR if needed in future.
Historically shred deduping was done outside the thread-pool because the deduper required a mutable reference and/or locking. Current deduper uses atomic operations and can be used inside the thread-pool without creating contention.
behzadnouri
force-pushed
the
retransmit-par-dedup
branch
from
6b5c516 to
47c84f7
Compare
Problem
Historically shred deduping was done outside the thread-pool because the deduper required a mutable refrences and/or locking. Current deduper uses atomic operations and can be used inside the thread-pool without creating contention.
Summary of Changes